feat(keep): --max-latency-ms as a constraint on every promotion - #1297
feat(keep): --max-latency-ms as a constraint on every promotion#1297rpoornac wants to merge 2 commits into
Conversation
87d467e to
cfb2428
Compare
CI E2E report — ✅ Succeeded
|
|
The optimizer maximized output_throughput and nothing else. Latency was measured, reported and fed to the prompts, but no latency number could block a KEEP. That is survivable for a lever that raises throughput without touching per-request latency, and unsafe for any lever that raises throughput *by* making each stream slower: against a throughput-only gate such a lever does not merely tolerate a latency regression, it selects for the largest one on offer. --max-latency-ms names a ceiling on mean end-to-end latency. It is a constraint rather than a target, so it sits outside the --target-* mutually-exclusive group and combines with them. Enforcement is at _lift_to_current_best, the single choke point that writes current_best, so it holds for explore, kernel, specialist and integrate winners alike rather than only for the lane wired first. Explore applies it a round earlier too, which keeps an over-budget variant from being folded onto the stack and becoming the anchor the rest of the batch is graded against, and gives the ledger a latency reason for the REVERT rather than a promotion refused later with no round to attribute it to. Off by default, leaving KEEP behaviour exactly as it was. When set the gate fails closed: a candidate that reported no end-to-end latency is refused, since an unmeasured constraint is not a satisfied one. The lift dicts that lanes hand to the choke point carry their measured latency so that fail-closed rule refuses the untimed rather than the unplumbed, and latency_from_result centralizes the spellings of the field in flight across the executors. Refusals are recorded on SharedState and listed in the report: a constrained session that ends near its baseline is otherwise indistinguishable from one that found no headroom, and the two call for opposite responses. A baseline already over budget warns rather than failing, since it is the reference the run is measured against. Co-authored-by: Cursor <cursoragent@cursor.com>
A fail-closed gate is only as good as what reaches it, and the bench lanes reached it with nothing. integrate_patch and the specialist rebench build their result dicts with no end-to-end latency at all, and report the other two figures under the GEAK spellings (ttft_ms / itl_ms) that the breakdown collectors read -- so the gate, which looked only for the canonical name on the top level, saw an untimed candidate. Under any --max-latency-ms that is not a cosmetic gap: the gate refuses the untimed by design, so those lanes lost every KEEP they would ever have made, whatever their latency, visible only as a log.warning. Two halves. Those dicts now carry end-to-end latency, in the same spelling as the siblings beside them so the collector contract is untouched. And latency_fields_from_result normalizes what promotion reads: the spellings in flight, plus the one layer of nesting integrate_patch uses when it returns the measurement under bench_result. The lift dicts go through it, so the gate grades the workload rather than the wiring, and current_best carries the latency it was graded on. The report claimed "within budget" without comparing. The gate exempts the baseline -- it is the reference the run is measured against, not a candidate -- so that slot can legitimately hold an over-budget figure, and the unconditional label asserted the opposite in the one place an operator would look to catch it. It now compares, and says plainly that an over-budget best config means nothing has yet come in under the SLA. orchestration.md told the model to read latency_refusals, which no prompt contained. SharedState.to_latency_budget_summary renders the constraint and its recent refusals as "=== Latency budget (constraint) ===", so the routing guidance points at something the model can see: under a budget a throughput gain no longer predicts a KEEP, and a growing refusal list means the SLA is binding rather than the search space exhausted. The resume path exported the budget before the resume branch, so with no flag it popped HYPERLOOM_MAX_LATENCY_MS before the restore could read it and the documented CLI > env > archived-state chain had a dead middle link. The export is now fresh-launch only, matching the workload envs beside it. Restoring from env also no longer hands an unparseable value straight to float(): a stale shell variable falls through to the archived budget rather than ending the resume. SKILL.md's flag table, documented as the source of truth for forwarding an operator's stated values, gains the --max-latency-ms row it was missing; without it a latency SLA stated in prompt form was dropped. Tests: 30 -> 57 in test_latency_budget.py. The new ones drive the lanes' real result shapes rather than a dict that already complies, pin the canonical names against a VariantResult rename, and cover the report comparison, the prompt block, and each tier of the resume precedence. Co-authored-by: Cursor <cursoragent@cursor.com>
cfb2428 to
4b765ed
Compare
|
All five addressed in 1. Fail-closed killed promotion for the bench lanes. Confirmed, and the shape is slightly different on current
On the test: agreed the old ones couldn't have caught this, since every one hardcoded 2. Report claimed "within budget" unconditionally. Fixed — it compares now. Against a 200 ms budget with 1211 ms kept it reads 3. 4. Resume cleared the env tier before restoring it. Guarded exactly as you suggested. Worth flagging: that fix exposed a latent crash one line further on — with the env no longer popped, an unparseable 5. What the rebase changed. One adjacent bug, already dead — no action. While confirming finding 1 I found the same class of bug in the retired lane's Verification. |
2 participants
Why
The optimizer maximized
output_throughputand nothing else. Latency wasmeasured, reported, and fed to the prompts, but no latency number could block a
KEEP.
That is survivable for a lever that raises throughput without touching
per-request latency. It is unsafe for any lever that raises throughput by
making each stream slower — against a throughput-only gate, such a lever does
not merely tolerate a latency regression, it selects for the largest one on
offer.
What
--max-latency-msnames a ceiling on mean end-to-end latency. It is aconstraint, not a target, so it sits outside the
--target-*mutually-exclusivegroup and combines with them.
Enforcement lives at
_lift_to_current_best, the single choke point that writescurrent_best. That means it holds for explore, kernel, specialist and integratewinners alike, rather than only for the lane that got wired first. Explore
applies it a round earlier as well, which keeps an over-budget variant from being
folded onto the stack and becoming the anchor the rest of the batch is graded
against, and gives the ledger a latency reason for the REVERT instead of a
promotion refused later with no round to attribute it to.
Fail-closed
Off by default; KEEP behaviour is exactly as it was when unset.
When set, the gate fails closed: a candidate that reported no end-to-end latency
is refused, because an unmeasured constraint is not a satisfied one. This makes
latency part of the measurement contract for the promotion paths, so the lift
dicts each lane hands to the choke point now carry their measured latency — that
way the fail-closed rule refuses the untimed, not the unplumbed.
latency_fields_from_resultcentralizes both halves of that lookup: thespellings in flight across the executors (
e2el_mean_ms,mean_e2el_ms,e2el_ms, andttft_ms/itl_msfor the other two figures), and the one layerof nesting
integrate_patchuses when it returns its measurement underbench_result. A lane reporting a different alias, or reporting a layer down, istherefore not misread as unmeasured — the gate grades the workload rather than
the wiring.
The baseline is the one exception: it is the reference point, so an over-budget
baseline warns rather than refuses. It warns loudly, because every subsequent
candidate will then fail the gate. The report compares the kept configuration
against the ceiling rather than asserting it satisfies it, since that exempt
baseline can legitimately occupy the slot while over budget.
Refusals are recorded to
SharedState.latency_refusals(action, variant, tput,observed latency, budget, reason) and render into the orchestration prompt as
=== Latency budget (constraint) ===. Without that, a session that ends nearbaseline is indistinguishable from one that exhausted its headroom, and the
router cannot tell a binding SLA from an exhausted search space.
Tests
89 tests in
test_latency_budget.py: resolution precedence, the predicate, thefield aliases and nesting, call-site wiring for all seven promotion paths, the
lanes' real result shapes, the report comparison, the prompt block, and each tier
of the resume precedence.
Review notes
This was split out of #1288, where the flag existed but only
ExploreExecutorhonoured it while the help text claimed otherwise. It is orthogonal to the
partition work: the constraint applies to any throughput-for-latency trade, so
it is reviewable and useful on its own.
Rebased onto current
mainafter review. That dropped two pieces of the originaldiff whose machinery no longer exists upstream: the
StackRebenchResultlatencyfield (the post-KEEP confirmation round was removed) and the framework lane's
lift (the
framework_agentaction was retired; those candidates now promotethrough
integrate_patch, which carries the gate).